Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: improve performance for incoming headers #26041

Closed
wants to merge 4 commits into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Feb 11, 2019

This improves the performance of matchKnownFields. This function is heavily used when parsing incoming http headers.

Here is the benchmark result:

                                                                             confidence improvement accuracy (*)    (**)   (***)
 http/incoming_headers.js headerDuplicates=0 c=50 benchmarker='autocannon'                   0.53 %      ±3.11% ±4.16% ±5.44%
 http/incoming_headers.js headerDuplicates=0 c=500 benchmarker='autocannon'                  2.16 %      ±3.69% ±4.95% ±6.50%
 http/incoming_headers.js headerDuplicates=20 c=50 benchmarker='autocannon'         ***     19.92 %      ±3.53% ±4.69% ±6.12%
 http/incoming_headers.js headerDuplicates=20 c=500 benchmarker='autocannon'        ***     16.96 %      ±3.89% ±5.18% ±6.74%
 http/incoming_headers.js headerDuplicates=5 c=50 benchmarker='autocannon'          ***     10.68 %      ±3.57% ±4.76% ±6.19%
 http/incoming_headers.js headerDuplicates=5 c=500 benchmarker='autocannon'           *      6.26 %      ±4.86% ±6.47% ±8.42%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 11, 2019
@starkwang
Copy link
Contributor Author

@@ -137,130 +137,101 @@ function _addHeaderLines(headers, n) {

// 'array' header list is taken from:
// https://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpHeaderArray.cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment might be outdated now? The link is dead at any rate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link is redirected to here. But I think it might not be a good way to keep an unstable url in comment. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be okay with removing it.

if (lowercased) {
return '\u0000' + field;
} else {
return matchKnownFields(field.toLowerCase(), true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be slightly faster to use iteration than recursion. it might also be a wash, however. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run the benchmark of iteration and found there was no difference between them.

return 'expires';
case 'Set-Cookie':
case 'set-cookie':
if (field === 'If-Match' || field === 'if-Match')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to be if-match instead of if-Match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll change it.

@starkwang
Copy link
Contributor Author

@starkwang
Copy link
Contributor Author

Landed in da0dc51

@starkwang starkwang closed this Feb 13, 2019
pull bot pushed a commit to SimenB/node that referenced this pull request Feb 13, 2019
PR-URL: nodejs#26041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 13, 2019
PR-URL: #26041
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants